Handle ECH Retry - #9611

I’m walking through this, and trying to cover all our bases.

This sets up a mechanism to attempt the next connection spec in sequence, and it’s replaced with code to attempt the next ECH config.

If the ECH config fails, do we attempt the next connection spec? I suppose we don’t per the ECH doc, which is also weird!

If the server rejects ECH, the client proceeds with the handshake, authenticating for ECHConfig.contents.public_name as described in Section 6.1.7. If authentication or the handshake fails, the client MUST return a failure to the calling application.

(We shouldn’t fall back using our normal fallback mechanism, and that’s difficult to test)

I think we should clarify, one badly configured server/region is more likely than poorly attempted hacking, so I don't like reducing one of the "free" wins of distributed computing, retries.

But the change was really intended to just be to include the possible mismatch failure in the next choice, so it can't happen before connectTls.

Does this cover the fallback concern for you?

    // If this was already in response to an ech retry, we are done for this
    // connection
    if (echRetryConfig != null || !retryTlsHandshake(sslException)) return null

Yeah, I am mostly eager to write a bunch of test cases for this because the logic is too difficult for me to follow statically.

Note to self... this behavior isn’t removed, we just compute a null retryTlsConnection elsewhere

This condition surprises me. The logic looks incorrect for the case where the server securely disables ECH. In particular I would expect the offeredEchRetryConfig to be null in the case where the server rejects ECH (distinct from offeredEchRetryConfig.configList == null)

From the validation comments here #9611 (comment)

Which one do you think is which?

I think it might be a problem with documentation and naming, and not a problem with logic.

If the server doesn’t include an ECH extension, there’s no ‘offered ECH retry config’. In particular there shouldn’t be anything offered there, perhaps derived.

But we do need an object to extract the public name from the EchConfigList object we passed in.

(The class EchRetryConfig is documented as sent by a server but I don’t think the publicName can be sent by a server; that needs to come from our passed-in EchConfigList)

This doc says ‘Sent by a server’ but we need a mechanism to lookup the public name when the server replies without this extension.

Does this class represent the encrypted_client_hello extension? Or is it a higher-level object that describes the TLS client’s state?

(I think it’s probably something describing the client state, because I don’t think the publicHostname is included in the encrypted_client_hello extension)

https://cs.android.com/android/platform/superproject/+/android-latest-release:external/boringssl/src/include/openssl/ssl.h;l=4352?q=SSL_get0_ech_name_override&ss=android%2Fplatform%2Fsuperproject:external%2F&start=11

// SSL_get0_ech_name_override, if |ssl| is a client and the server rejected ECH,
// sets |*out_name| and |*out_name_len| to point to a buffer containing the ECH
// public name. Otherwise, the buffer will be empty.
//
// When offering ECH as a client, this function should be called during the
// certificate verification callback (see |SSL_CTX_set_custom_verify|). If
// |*out_name_len| is non-zero, the caller should verify the certificate against
// the result, interpreted as a DNS name, rather than the true server name. In
// this case, the handshake will never succeed and is only used to authenticate
// retry configs. See also |SSL_get0_ech_retry_configs|.
OPENSSL_EXPORT void SSL_get0_ech_name_override(const SSL *ssl,
                                               const char **out_name,
                                               size_t *out_name_len);

  std::string_view ech_name_override = GetECHNameOverride();
  if (!ech_name_override.empty()) {
    // If ECH was offered but not negotiated, BoringSSL will ask to verify a
    // different name than the origin. If verification succeeds, we continue the
    // handshake, but BoringSSL will not report success from SSL_do_handshake().
    // If all else succeeds, BoringSSL will report |SSL_R_ECH_REJECTED|, mapped
    // to |ERR_R_ECH_NOT_NEGOTIATED|. |ech_name_override| is only used to
    // authenticate GetECHRetryConfigs().
    DCHECK(!ssl_config_.ech_config_list.empty());
    used_ech_name_override_ = true;